-
Notifications
You must be signed in to change notification settings - Fork 769
SOLR-17931: Fix getSolrClientCache() to leverage ObjectCache #3740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
* @deprecated likely to simply be moved to the ObjectCache so as to not be used | ||
*/ | ||
@Deprecated | ||
public SolrClientCache getSolrClientCache() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No; this misses the point. I don't like this method's existence; it's too tempting to use it when usually you shouldn't use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate more on what the problem with this method is? Is there a more general approach that we are supposed to take for getting clients? at least to me, it appears super useful... If we we have a client created, great, use it, and if not, it creates it...
Only six classes call it, so if we want to eliminate it, it doesn't seem unsurmountable. Just unclear on what we would replace it with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the javadocs don't already answer your question, I failed to make them clear enough. Let me flip this inquiry around... hey Eric, why do you think we need a cache of clients at all? Why cache them? What's wrong with our existing clients (linked to in javadocs)?
note: distributed file store usage could easily switch to coreContainer.getDefaultHttpSolrClient().requestWithBaseUrl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, after re-reading and more context.. ( I sound like a LLM!) I think what you are saying is that we could replace most calls with one of:
* @see #getDefaultHttpSolrClient()
* @see ZkController#getSolrClient()
* @see Http2SolrClient#requestWithBaseUrl(String, String, SolrRequest)
and that would work? I'll take a looksee at that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also fine with the PR, TBH... enough callers of it want a SolrClientCache that it'd be annoying to outright remove it. And it's not a big deal if a plugin or something uses it when they could have used an alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so coreContainer.getDefaultHttpSolrClient().requestWithBaseUrl
already is used in some places in DistribFileStore. However we do:
final var fileResponse = solrClient.requestWithBaseUrl(baseUrl, null, fileRequest);
try (final var stream = fileResponse.getResponseStreamIfSuccessful()) {
can't quite figure that getResponseStreamIfSucessful to work yet
…aultHttpSolrClient Reduce the exposure of getSolrClientCache.
https://issues.apache.org/jira/browse/SOLR-17931
Description
Deal with deprecation tag.
Solution
Move SolrClientCache to live in the ObjectCache, which is the path recommended by the deprecation tag.
I left in a lot of the comments from Claude, however would prune it down if this patch looks good to folks.
Tests
No new tests, re-ran existing test.